fix: separate filter scan height from synced height#442
fix: separate filter scan height from synced height#442xdustinface merged 2 commits intov0.42-devfrom
Conversation
📝 WalkthroughWalkthroughReplaces wallet "synced height" usage with a new "filter committed height" concept across filter sync logic and wallet manager. Adds trait accessors and storage for filter_committed_height, updates initialization, progress checks, and commit paths to use committed height for filter batch lifecycle and restart recovery. Changes
Sequence DiagramsequenceDiagram
participant FM as Filter Manager
participant WI as Wallet Interface
participant WM as Wallet Manager
FM->>WI: filter_committed_height()
WI->>WM: filter_committed_height()
WM-->>WI: return committed_height
WI-->>FM: return committed_height
FM->>FM: calculate scan_start / decide batches from committed_height
FM->>FM: process filter batch
FM->>WI: update_filter_committed_height(batch.end_height)
WI->>WM: update_filter_committed_height(height)
WM->>WM: set filter_committed_height = height
WM->>WM: if height > synced_height -> update_synced_height(height)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@dash-spv/src/sync/filters/manager.rs`:
- Around line 496-500: When committing the batch you must avoid regressing
synced_height: before calling
wallet.write().await.update_filter_committed_height(batch.end_height()) (in the
commit block that removes active_batches and sets
self.committed_height/self.processing_height), read the current synced height
from the wallet and only update the committed/synced value if batch.end_height()
does not move synced_height backward (e.g. use max(current_synced_height,
batch.end_height()) or skip the update when batch.end_height() <
current_synced_height). Mirror the guard used in process_block.rs so
update_filter_committed_height cannot cause synced_height to decrease.
In `@key-wallet-manager/src/wallet_manager/process_block.rs`:
- Around line 140-143: The method update_filter_committed_height currently sets
filter_committed_height and then unconditionally calls update_synced_height,
which can regress the tracked synced_height; change it so that after setting
self.filter_committed_height = height you only call
self.update_synced_height(height) if the incoming height is greater than the
current self.synced_height (i.e., avoid backward movement), referencing
update_filter_committed_height, update_synced_height, and the fields
synced_height and filter_committed_height.
4a66511 to
bb48156
Compare
bb48156 to
7b60925
Compare
|
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
Introduces `filter_committed_height` on `WalletInterface`, separate from `synced_height`. `process_block` updates `synced_height` per-block (needed for balance/maturity calculations), but `FiltersManager` uses `filter_committed_height` for restart recovery. This prevents a bug where per-block `synced_height` advances past uncommitted filter batches, causing the rescan on restart to skip heights that need rescanning for newly discovered addresses. It's not the best solution I think, hence I added the TODO for now. I will look into this at some point later.
Sync completion checks in `start_download` and `check_sync_complete` used `progress.current_height()` which tracks stored filter height, not the height committed to wallet. This could lead to sync complete before all batches were committed, causing skipped heights on restart.
7b60925 to
f563e33
Compare
|
@coderabbitai review (note this was the human, logged into the wrong account) |
✅ Actions performedReview triggered.
|
|
🧠 Learnings used✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
dash-spv/src/sync/filters/manager.rs (1)
151-156: Verifyscan_startcalculation whenwallet_committed_height > 0but less thanwallet_birth_height.If
wallet_committed_heightis between 1 andwallet_birth_height - 1(e.g., due to data corruption or manual reset), thenwallet_birth_height.max(wallet_committed_height + 1)correctly resolves towallet_birth_height, so this is safe. However, this means a committed height below birth height is silently ignored — if this scenario is unexpected, a warning log could help debugging.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash-spv/src/sync/filters/manager.rs` around lines 151 - 156, The calculation of scan_start using wallet_birth_height.max(wallet_committed_height + 1).max(header_start_height) can silently ignore a non-zero wallet_committed_height that is less than wallet_birth_height; add an explicit check before computing scan_start: if wallet_committed_height > 0 && wallet_committed_height < wallet_birth_height, emit a warning log (via the existing logger used in this module) indicating the unexpected committed height and the values of wallet_committed_height and wallet_birth_height, then proceed with the current max-based computation; reference the variables scan_start, wallet_committed_height, wallet_birth_height, and header_start_height to locate where to insert the check and logging.key-wallet-manager/src/wallet_manager/process_block.rs (1)
184-197: Consider adding test coverage forfilter_committed_height.The existing
test_synced_heighttest validatessynced_heightbut doesn't exercise the newfilter_committed_heightmethods or verify the interaction between them (e.g., thatupdate_filter_committed_heightonly advancessynced_heightwhen the new height exceeds it).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-manager/src/wallet_manager/process_block.rs` around lines 184 - 197, Add a new async test (e.g., test_filter_committed_height) that constructs a WalletManager<ManagedWalletInfo> (Network::Testnet), asserts initial values for synced_height() and filter_committed_height(), then call update_filter_committed_height with increasing heights and verify that filter_committed_height() advances accordingly and that synced_height() only advances when the new filter committed height exceeds the current synced_height (and never decreases when a smaller value is applied). Also include a case where update_filter_committed_height is called with a lower height to ensure neither filter_committed_height nor synced_height decreases, and a case where advancing filter_committed_height causes synced_height to be bumped to the same value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@dash-spv/src/sync/filters/manager.rs`:
- Around line 500-507: The commit logic already prevents regressing
committed_height: when removing a batch via active_batches.remove(&batch_start)
and computing end = batch.end_height(), the code only updates
self.committed_height and calls
wallet.write().await.update_filter_committed_height(end) if end >
self.committed_height, and processing_height is advanced to end + 1—no code
change required; keep the existing guard around committed_height and the call to
update_filter_committed_height as-is (they together prevent backward movement).
---
Nitpick comments:
In `@dash-spv/src/sync/filters/manager.rs`:
- Around line 151-156: The calculation of scan_start using
wallet_birth_height.max(wallet_committed_height + 1).max(header_start_height)
can silently ignore a non-zero wallet_committed_height that is less than
wallet_birth_height; add an explicit check before computing scan_start: if
wallet_committed_height > 0 && wallet_committed_height < wallet_birth_height,
emit a warning log (via the existing logger used in this module) indicating the
unexpected committed height and the values of wallet_committed_height and
wallet_birth_height, then proceed with the current max-based computation;
reference the variables scan_start, wallet_committed_height,
wallet_birth_height, and header_start_height to locate where to insert the check
and logging.
In `@key-wallet-manager/src/wallet_manager/process_block.rs`:
- Around line 184-197: Add a new async test (e.g., test_filter_committed_height)
that constructs a WalletManager<ManagedWalletInfo> (Network::Testnet), asserts
initial values for synced_height() and filter_committed_height(), then call
update_filter_committed_height with increasing heights and verify that
filter_committed_height() advances accordingly and that synced_height() only
advances when the new filter committed height exceeds the current synced_height
(and never decreases when a smaller value is applied). Also include a case where
update_filter_committed_height is called with a lower height to ensure neither
filter_committed_height nor synced_height decreases, and a case where advancing
filter_committed_height causes synced_height to be bumped to the same value.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
key-wallet-manager/src/wallet_manager/process_block.rs (1)
183-197: Consider adding test coverage for the newfilter_committed_heightmethods.The existing
test_synced_heighttest validatessynced_heightbehavior but doesn't cover the newfilter_committed_height/update_filter_committed_heightmethods. A small test verifying the decoupled behavior (e.g., thatupdate_filter_committed_heightdoesn't regresssynced_height) would increase confidence.💡 Example test
#[tokio::test] async fn test_filter_committed_height() { let mut manager: WalletManager<ManagedWalletInfo> = WalletManager::new(Network::Testnet); assert_eq!(manager.filter_committed_height(), 0); // Advance synced_height via process_block simulation manager.update_synced_height(1000); assert_eq!(manager.synced_height(), 1000); // filter_committed_height should be independent assert_eq!(manager.filter_committed_height(), 0); // Committing a lower filter height should NOT regress synced_height manager.update_filter_committed_height(500); assert_eq!(manager.filter_committed_height(), 500); assert_eq!(manager.synced_height(), 1000); // unchanged // Committing a higher filter height should also advance synced_height manager.update_filter_committed_height(2000); assert_eq!(manager.filter_committed_height(), 2000); assert_eq!(manager.synced_height(), 2000); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-manager/src/wallet_manager/process_block.rs` around lines 183 - 197, Add a new unit test mirroring test_synced_height to cover the new filter_committed_height behavior on WalletManager<ManagedWalletInfo>: verify initial filter_committed_height() == 0, that calling update_synced_height(...) does not change filter_committed_height, that update_filter_committed_height(lower) updates filter_committed_height without regressing synced_height(), and that update_filter_committed_height(higher) advances both filter_committed_height() and synced_height() as expected; place the test alongside test_synced_height in process_block.rs and reference the WalletManager, update_synced_height, update_filter_committed_height, filter_committed_height, and synced_height methods.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@key-wallet-manager/src/wallet_manager/process_block.rs`:
- Around line 183-197: Add a new unit test mirroring test_synced_height to cover
the new filter_committed_height behavior on WalletManager<ManagedWalletInfo>:
verify initial filter_committed_height() == 0, that calling
update_synced_height(...) does not change filter_committed_height, that
update_filter_committed_height(lower) updates filter_committed_height without
regressing synced_height(), and that update_filter_committed_height(higher)
advances both filter_committed_height() and synced_height() as expected; place
the test alongside test_synced_height in process_block.rs and reference the
WalletManager, update_synced_height, update_filter_committed_height,
filter_committed_height, and synced_height methods.
Introduces
filter_committed_heightonWalletInterface, separate fromsynced_height.process_blockupdatessynced_heightper-block (needed for balance/maturity calculations), butFiltersManagerusesfilter_committed_heightfor restart recovery. This prevents a bug where per-blocksynced_heightadvances past uncommitted filter batches, causing the rescan on restart to skip heights that need rescanning for newly discovered addresses.It's not the best solution I think, hence I added the TODO for now. I will look into this at some point later.
Summary by CodeRabbit